Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix request abort for node 16+ #1580

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Matt-Esch
Copy link

After upgrading to node 16, the web proxy stopped working when requests were aborted. The specific use case was SSE where the incoming request has ended but the server response is a long lived http connection. The 'aborted' event and the 'abort' method are deprecated. The response 'close' event and the 'destroy' method are the correct alternatives and pre-date node 8, which is the minimum version this package currently supports. I did run tests on node 8/10/12/14/16 and they failed on node 16 prior to this fix and all pass after this fix.

likev added a commit to likev/node-http-proxy that referenced this pull request Feb 23, 2023
@anthonyalayo
Copy link

Can we merge this?

@callumgare
Copy link

callumgare commented May 12, 2023

Unfortunately won't work in node.js 16 and below since the "close" event was added to in v16:

https://nodejs.org/docs/latest-v16.x/api/http.html#event-close
https://nodejs.org/docs/latest-v15.x/api/http.html#event-close

Can't find any info on what's the minimum node version node-http-proxy supports but in this case it should be pretty easy to use "abort" for v15 and below and "close" for v16 and above. That's what #1559 does

@gdrbyKo1
Copy link

@callumgare I'm not sure I follow. This PR attaches a "close" event listener to res, which is an instance of http.ServerResponse. According to Node.js docs, this event is present in both v16 and v15:
https://nodejs.org/docs/latest-v16.x/api/http.html#event-close_2
https://nodejs.org/docs/latest-v15.x/api/http.html#http_event_close_1
Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants